Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define error hint for insert on DynamicIndexLens. #140

Merged
merged 6 commits into from
Feb 28, 2024

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Feb 27, 2024

Alternative to #132

Here's what it looks like when a user hits the error:
image

I went kinda maximalist on the description, but if we want we can slim it down a lot.

@jw3126
Copy link
Member

jw3126 commented Feb 27, 2024

Awesome! I like the message, as is. However if you want to shorten it one option would be to link the discussion in your other PR.

@MasonProtter
Copy link
Contributor Author

Interesting, I wonder how this is causing failures on v1.6

@jw3126
Copy link
Member

jw3126 commented Feb 27, 2024

yikes, I think we should then drop the hint on v1.6.

@aplavin
Copy link
Member

aplavin commented Feb 27, 2024

Great! Both for this specific hint itself, and for setting the precedent :)
I can easily see more error hints added to Accessors over time to make errors clearer.

Maybe, in the future we should turn methods that exist only to show user-friendly errors into error hints (eg

Accessors.jl/src/optics.jl

Lines 177 to 183 in dc42b02

function _modify(f, obj, optic, ::ModifyBased)
Optic = typeof(optic)
error("""
This should be unreachable. You probably need to overload:
`Accessors.modify(f, obj, ::$Optic)`
""")
end
)...

@jw3126
Copy link
Member

jw3126 commented Feb 27, 2024

Maybe, in the future we should turn methods that exist only to show user-friendly errors into error hints (eg

What are the pros and cons for having an error hint vs a detailed error message?

@@ -10,6 +10,7 @@ Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
InverseFunctions = "3587e190-3f89-42d0-90ee-14403ec27112"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09"
Markdown = "d6f4376e-aef5-505a-96c1-9c027394607a"
Copy link
Member

@aplavin aplavin Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I myself will use Accessors even with more deps, but not sure what others think.
Note that several existing deps can easily be turned into extensions, if not for the Julia ext precompilation bug. If that happens, only 4 actual deps would remain.

Personally, I think the (shortened) error message won't lose much without markdown.
Are there any existing markdown error hints in Julia?

But this is a minor point, fine either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW Markdown is a stdlib so it's not particularly costly or weird to rely on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I can remove it too. I just like that it makes code snippets look a little distinct, and the REPL examples get syntax highlighting.

Copy link
Member

@aplavin aplavin Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe with this PR we should set the precedent for long and nicely formatted errorhints in Julia generally :) At least the Base ones are pretty minimal.

@aplavin
Copy link
Member

aplavin commented Feb 27, 2024

What are the pros and cons for having an error hint vs a detailed error message?

Don't know for certain, separation of concerns maybe?
Even in this specific example, one could easily make it an extra method throwing an error. Same as with Base hints like string + string or max(vector).

@jw3126
Copy link
Member

jw3126 commented Feb 27, 2024

Don't know for certain, separation of concerns maybe?

Mhh which concerns are separated here?

@MasonProtter
Copy link
Contributor Author

One problem with defining a method and then making it error is that it makes interactively exploring an API (or just trying to look-before-you-leap) less reliable.

e.g. if we did

insert(v, ::DynamicIndexLens, x)  = error("...")

then someone might do hasmethod(insert, ...) and get true, or to look at the list of methods(insert) and then think from that that the method is callable.

@aplavin
Copy link
Member

aplavin commented Feb 27, 2024

Mhh which concerns are separated here?

One is "arguments for the function are wrong" (then throw an error), another is "here's what you could do if we guess your intent correctly" (show an errorhint).

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Feb 27, 2024

One other reason that it's good to use error hints here rather than defining an error method is that defining error methods can cause method ambiguities. For example, if someone else defined the method

insert(::MyType, ::Any, ::Any) = ...

then it might end up being ambiguous with

insert(::Any, ::DynamicIndexLens, ::Any) = error("...")

even though the conflicting method is just a junk method we created to try and give more informative error messages

@MasonProtter
Copy link
Contributor Author

Looks like the v1.6 failure is unrelated?

@aplavin
Copy link
Member

aplavin commented Feb 27, 2024

Weird, it definitely passes on master.

@aplavin
Copy link
Member

aplavin commented Feb 27, 2024

Aaah, I see! There's another __init__ defined above :) And your method overwrites it.

@MasonProtter
Copy link
Contributor Author

Silly me.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Feb 27, 2024

An alternative design we could do here is something like this:
image

That keeps the hint small, but lets users interactively access the full error hint (it'd also let us drop the Markdown dependency, since docstrings handle markdown for us already)

@aplavin
Copy link
Member

aplavin commented Feb 28, 2024

I think it's better to avoid overengineering, and keep a single message in a single place.

As I understand, @jw3126 also likes the PR in its current state, so going to merge.
And looking forward for the bright future with understandable error messages :)

@aplavin aplavin merged commit e0ab153 into JuliaObjects:master Feb 28, 2024
6 of 8 checks passed
@aplavin
Copy link
Member

aplavin commented Mar 3, 2024

What are the pros and cons for having an error hint vs a detailed error message?

Oh, I think an even more concrete pro of an error hint is that it goes in addition to the actual error message – not replaces it.

Currently:

julia> @set f((a=1,b=2)) = 3
ERROR: This should be unreachable. You probably need to overload
`Accessors.set(obj, ::typeof(f), val)

Potentially without error() but with a hint with the same message:

julia> @set f((a=1,b=2)) = 3
ERROR: MethodError: no method matching _set(::@NamedTuple{a::Int64, b::Int64}, ::typeof(f), ::Int64, ::Accessors.SetBased)

You probably need to overload `Accessors.set(obj, ::typeof(f), val)

Closest candidates are:
  _set(::Any, ::Any, ::Any, ::Accessors.ModifyBased)
   @ Accessors ~/.julia/dev/Accessors/src/optics.jl:187
  _set(::Any, ::ComposedFunction, ::Any, ::Accessors.SetBased)
   @ Accessors ~/.julia/dev/Accessors/src/optics.jl:191

It shows the same suggestion, but also prints which specific method was not found (ie what type obj and val are), and closest candidates. The latter isn't really useful in this specific case, because the function _set is an internal one.
Actually, I don't know why OpticStyle/_set/_modify are needed at all. Is it just because Julia doesn't compile efficiently otherwise, or for some fundamental reasons?

@jw3126
Copy link
Member

jw3126 commented Mar 3, 2024

I don't know why OpticStyle/_set/_modify are needed at all. Is it just because Julia doesn't compile efficiently otherwise, or for some fundamental reasons?

These were needed a long time ago for inference reasons, which may no longer apply I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants